Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OpenMP] [cmake] Don't use -fno-semantic-interposition on Windows #81113

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

mstorsjo
Copy link
Member

@mstorsjo mstorsjo commented Feb 8, 2024

This was added in 4b7beab. When the flag was added implicitly elsewhere, it was added via llvm/cmake/modules/HandleLLVMOptions.cmake, where it wasn't added on Windows/Cygwin targets.

This avoids one warning per object file in OpenMP.

This was added in 4b7beab. When
the flag was added implicitly elsewhere, it was added via
llvm/cmake/modules/HandleLLVMOptions.cmake, where it wasn't added
on Windows/Cygwin targets.

This avoids one warning per object file in OpenMP.
@mstorsjo mstorsjo added cmake Build system in general and CMake in particular openmp labels Feb 8, 2024
@mstorsjo mstorsjo requested a review from jhuber6 February 8, 2024 09:49
@llvmbot llvmbot added openmp:libomp OpenMP host runtime openmp:libomptarget OpenMP offload runtime labels Feb 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Feb 8, 2024

@llvm/pr-subscribers-openmp

Author: Martin Storsjö (mstorsjo)

Changes

This was added in 4b7beab. When the flag was added implicitly elsewhere, it was added via llvm/cmake/modules/HandleLLVMOptions.cmake, where it wasn't added on Windows/Cygwin targets.

This avoids one warning per object file in OpenMP.


Full diff: https://github.com/llvm/llvm-project/pull/81113.diff

1 Files Affected:

  • (modified) openmp/cmake/HandleOpenMPOptions.cmake (+5-1)
diff --git a/openmp/cmake/HandleOpenMPOptions.cmake b/openmp/cmake/HandleOpenMPOptions.cmake
index 71346201129b68..9387d9b3b0ff75 100644
--- a/openmp/cmake/HandleOpenMPOptions.cmake
+++ b/openmp/cmake/HandleOpenMPOptions.cmake
@@ -46,7 +46,11 @@ append_if(OPENMP_HAVE_WEXTRA_FLAG "-Wno-extra" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 append_if(OPENMP_HAVE_WPEDANTIC_FLAG "-Wno-pedantic" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 append_if(OPENMP_HAVE_WMAYBE_UNINITIALIZED_FLAG "-Wno-maybe-uninitialized" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 
-append_if(OPENMP_HAVE_NO_SEMANTIC_INTERPOSITION "-fno-semantic-interposition" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+if (NOT (WIN32 OR CYGWIN))
+  # This flag is not relevant on Windows; the flag is accepted, but produces warnings
+  # about argument unused during compilation.
+  append_if(OPENMP_HAVE_NO_SEMANTIC_INTERPOSITION "-fno-semantic-interposition" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+endif()
 append_if(OPENMP_HAVE_FUNCTION_SECTIONS "-ffunction-section" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 append_if(OPENMP_HAVE_DATA_SECTIONS "-fdata-sections" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could likely remove this entire thing until we figure out a better way to handle this. It seems our options handling is kind of a mess right now and doesn't do a good job separating runtime builds from the LLVM project itself. But this is fine.

@mstorsjo
Copy link
Member Author

mstorsjo commented Feb 8, 2024

You could likely remove this entire thing until we figure out a better way to handle this. It seems our options handling is kind of a mess right now and doesn't do a good job separating runtime builds from the LLVM project itself. But this is fine.

Thanks! Yes, openmp doesn't work quite smoothly in all these build settings. Contrary to other runtimes such as libcxx, there's actually 4 different ways one can build openmp; building the main llvm directory, including openmp in LLVM_ENABLE_PROJECTS (which afaik is probably the most default and tested way), including openmp in LLVM_ENABLE_RUNTIMES. Then one can also build it standalone by pointing cmake at llvm-project/runtimes and including openmp in LLVM_ENABLE_RUNTIMES. And finally, one can build it by pointing cmake directly at llvm-project/openmp (which is mostly supported and tested, but some things break occasionally).

In the case of libcxx, it only supports the two ways where it is specified via LLVM_ENABLE_RUNTIMES (either via llvm-project/llvm or by directly pointing at llvm-project/runtimes).

Anyway, would you be fine with backporting this to 18.x, to reduce the amount of build time warnings there?

@mstorsjo mstorsjo merged commit 72f04fa into llvm:main Feb 8, 2024
9 checks passed
@mstorsjo mstorsjo deleted the openmp-no-semantic-interp-win branch February 8, 2024 13:28
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 9, 2024
…vm#81113)

This was added in 4b7beab. When the
flag was added implicitly elsewhere, it was added via
llvm/cmake/modules/HandleLLVMOptions.cmake, where it wasn't added on
Windows/Cygwin targets.

This avoids one warning per object file in OpenMP.

(cherry picked from commit 72f04fa)
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 9, 2024
…vm#81113)

This was added in 4b7beab. When the
flag was added implicitly elsewhere, it was added via
llvm/cmake/modules/HandleLLVMOptions.cmake, where it wasn't added on
Windows/Cygwin targets.

This avoids one warning per object file in OpenMP.

(cherry picked from commit 72f04fa)
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 16, 2024
…vm#81113)

This was added in 4b7beab. When the
flag was added implicitly elsewhere, it was added via
llvm/cmake/modules/HandleLLVMOptions.cmake, where it wasn't added on
Windows/Cygwin targets.

This avoids one warning per object file in OpenMP.

(cherry picked from commit 72f04fa)
@pointhex pointhex mentioned this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Build system in general and CMake in particular openmp:libomp OpenMP host runtime openmp:libomptarget OpenMP offload runtime openmp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants